buffer: use fast API for writing one-byte strings #54311

ronag · 2024-08-10T19:59:03Z

                                                                        confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='ascii'          ***     65.81 %       ±5.23%  ±7.49% ±10.96%
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='latin1'         ***     58.19 %      ±20.88% ±29.89% ±43.73%
buffers/buffer-write-string-short.js n=1000000 len=0 encoding='utf8'           ***     61.74 %      ±13.09% ±18.14% ±25.14%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='ascii'          ***    136.24 %      ±11.25% ±15.57% ±21.57%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='latin1'         ***    135.68 %      ±34.53% ±49.25% ±71.59%
buffers/buffer-write-string-short.js n=1000000 len=1 encoding='utf8'           ***    131.19 %      ±13.69% ±19.05% ±26.59%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='ascii'         ***    105.31 %      ±22.74% ±32.63% ±47.91%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='latin1'        ***    146.98 %      ±15.12% ±20.88% ±28.83%
buffers/buffer-write-string-short.js n=1000000 len=16 encoding='utf8'          ***    231.98 %      ±39.44% ±56.50% ±82.72%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='ascii'         ***    119.88 %      ±17.20% ±23.74% ±32.73%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='latin1'        ***    127.78 %      ±14.96% ±21.09% ±30.09%
buffers/buffer-write-string-short.js n=1000000 len=32 encoding='utf8'          ***    286.10 %      ±36.29% ±51.94% ±75.93%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='ascii'          ***    126.35 %      ±20.58% ±28.48% ±39.41%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='latin1'         ***    155.13 %       ±8.24% ±11.43% ±15.90%
buffers/buffer-write-string-short.js n=1000000 len=8 encoding='utf8'           ***    200.61 %      ±33.84% ±48.37% ±70.55%

anonrig · 2024-08-10T20:01:28Z

Probably because the benchmark has a flaw, and the input is not a one byte string. I recommend reading this amazing blog post: https://iliazeus.github.io/articles/js-string-optimizations-en/

ronag · 2024-08-10T20:38:49Z

@anonrig still only minimal difference:

buffers/buffer-write-string.js n=1000000 len=1                  9.21 %      ±12.42% ±17.25% ±24.00%
buffers/buffer-write-string.js n=1000000 len=16          *      5.12 %       ±4.58%  ±6.53%  ±9.52%
buffers/buffer-write-string.js n=1000000 len=32                 2.87 %       ±8.49% ±12.18% ±17.88%
buffers/buffer-write-string.js n=1000000 len=8          **      8.27 %       ±5.40%  ±7.68% ±11.10%

anonrig · 2024-08-10T20:40:00Z

Can you add a printf to fast api function and make sure that it runs? I've had several occasions that I couldn't trigger fast path for super simple calls.

ronag · 2024-08-11T06:08:12Z

@anonrig You are right. It's never called and I have no idea why. Do you know anyone that might know more?

ronag · 2024-08-11T06:31:06Z

Ah, it's because buffer is this and not the first arg. Do you know if fast API works with this args?

targos · 2024-08-11T06:35:20Z

The first arg (receiver) is the this arg.

ronag · 2024-08-11T06:39:03Z

Tried with no this and still no success... seems like a dead end for me

lib/buffer.js

santigimeno · 2024-08-11T10:32:03Z

This works for me and the fast api path is hit starting around 1e4 iterations in the benchmark:

diff --git a/lib/buffer.js b/lib/buffer.js
index 37051c94073..cd4f8f296a1 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -54,8 +54,9 @@ const {
   Uint8ArrayPrototype,
 } = primordials;
 
+const bufferBinding = internalBinding('buffer');
+
 const {
-  stringWrite,
   byteLengthUtf8,
   compare: _compare,
   compareOffset,
@@ -658,7 +659,7 @@ const encodingOps = {
     encoding: 'ascii',
     encodingVal: encodingsMap.ascii,
     byteLength: (string) => string.length,
-    write: (buf, string, offset, len) => stringWrite(buf, string, offset, len),
+    write: (buf, string, offset, len) => bufferBinding.stringWrite(buf, string, offset, len),
     slice: (buf, start, end) => buf.asciiSlice(start, end),
     indexOf: (buf, val, byteOffset, dir) =>
       indexOfBuffer(buf,
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 3247797dec4..acf2677d65a 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -1422,6 +1422,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(StringSlice<UCS2>);
   registry->Register(StringSlice<UTF8>);
 
+  registry->Register(SlowWriteString<ASCII>);
   registry->Register(fast_write_string.GetTypeInfo());
   registry->Register(FastWriteString);
   registry->Register(StringWrite<ASCII>);

ronag · 2024-08-11T14:06:12Z

Got it to work!

buffers/buffer-write-string.js n=1000000 str='a'           ***    306.67 %       ±7.42% ±10.17% ±13.87%
buffers/buffer-write-string.js n=1000000 str='aa'          ***    289.45 %       ±3.26%  ±4.59%  ±6.55%
buffers/buffer-write-string.js n=1000000 str='aaaa'        ***    271.79 %      ±12.09% ±16.96% ±24.00%

looks promising

nodejs-github-bot · 2024-08-15T01:01:07Z

Landed in ccf05ef

ShenHongFei · 2024-08-15T03:01:33Z

Access violation occurred in FastWriteString function, causing the process to crash and exit. @ronag

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag · 2024-08-15T06:27:47Z

@ShenHongFei Thanks for reporting! Fix is ready.

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ShenHongFei · 2024-08-15T06:42:25Z

I tried applying the patch https://github.com/nodejs/node/pull/54391/files and the problem was solved. Thank you.

Refs: #54311 (comment) PR-URL: #54391 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>

PR-URL: #54310 PR-URL: #54311 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Refs: #54311 (comment) PR-URL: #54391 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: TODO

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452

PR-URL: #54310 PR-URL: #54311 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Refs: #54311 (comment) PR-URL: #54391 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>

Notable changes: buffer: * use fast API for writing one-byte strings (Robert Nagy) #54311 * optimize createFromString (Robert Nagy) #54324 * use native copy impl (Robert Nagy) #54087 inspector: * (SEMVER-MINOR) support `Network.loadingFailed` event (Kohei Ueno) #54246 lib: * (SEMVER-MINOR) rewrite AsyncLocalStorage without async_hooks (Stephen Belanger) #48528 module: * (SEMVER-MINOR) add --experimental-transform-types flag (Marco Ippolito) #54283 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 PR-URL: #54452

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 10, 2024

targos approved these changes Aug 11, 2024

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

ronag force-pushed the write-string-fast-api branch 2 times, most recently from de22cf8 to 63e7cda Compare August 11, 2024 07:36

ronag force-pushed the write-string-fast-api branch 5 times, most recently from f3259aa to d1eb51b Compare August 11, 2024 14:25

ronag marked this pull request as ready for review August 11, 2024 14:26

ronag requested review from mcollina, benjamingr and anonrig August 11, 2024 14:26

ronag force-pushed the write-string-fast-api branch 2 times, most recently from 9c98f8f to 2596951 Compare August 11, 2024 14:29

ronag changed the title ~~Write string fast api~~ buffer: use fast API for writing one-byte strings Aug 11, 2024

anonrig approved these changes Aug 11, 2024

View reviewed changes

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024

ronag force-pushed the write-string-fast-api branch from 2596951 to 6594e2e Compare August 11, 2024 15:12

ronag requested review from BridgeAR and anonrig August 14, 2024 19:04

anonrig approved these changes Aug 14, 2024

View reviewed changes

jakecastelli added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 15, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2024

nodejs-github-bot merged commit ccf05ef into nodejs:main Aug 15, 2024
58 checks passed

This comment was marked as outdated.

Sign in to view

ronag mentioned this pull request Aug 15, 2024

buffer: properly apply dst offset and src length on fast path #54391

Merged

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply offset on fast path

ea45c6c

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply offset on fast path

9795353

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

ronag added a commit to nxtedition/node that referenced this pull request Aug 15, 2024

buffer: properly apply dst offset and src length on fast path

bbafef7

Refs: nodejs#54311 (comment) PR-URL: nodejs#54391

richardlau mentioned this pull request Aug 15, 2024

src: move more crypto code to ncrypto #54320

Closed

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

dlarocque mentioned this pull request Aug 23, 2024

Compatibility error with Node 22.7 firebase/firebase-js-sdk#8455

Closed

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: use fast API for writing one-byte strings #54311

buffer: use fast API for writing one-byte strings #54311

ronag commented Aug 10, 2024 •

edited

Loading

anonrig commented Aug 10, 2024

ronag commented Aug 10, 2024

anonrig commented Aug 10, 2024

ronag commented Aug 11, 2024

ronag commented Aug 11, 2024

targos commented Aug 11, 2024

ronag commented Aug 11, 2024

santigimeno commented Aug 11, 2024

ronag commented Aug 11, 2024

nodejs-github-bot commented Aug 15, 2024

ShenHongFei commented Aug 15, 2024

This comment was marked as outdated.

ronag commented Aug 15, 2024

ShenHongFei commented Aug 15, 2024

buffer: use fast API for writing one-byte strings #54311

buffer: use fast API for writing one-byte strings #54311

Conversation

ronag commented Aug 10, 2024 • edited Loading

anonrig commented Aug 10, 2024

ronag commented Aug 10, 2024

anonrig commented Aug 10, 2024

ronag commented Aug 11, 2024

ronag commented Aug 11, 2024

targos commented Aug 11, 2024

ronag commented Aug 11, 2024

santigimeno commented Aug 11, 2024

ronag commented Aug 11, 2024

nodejs-github-bot commented Aug 15, 2024

ShenHongFei commented Aug 15, 2024

This comment was marked as outdated.

ronag commented Aug 15, 2024

ShenHongFei commented Aug 15, 2024

ronag commented Aug 10, 2024 •

edited

Loading